Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OP_PAIRCOMMIT #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

moonsettler
Copy link

No description provided.

Use CSFS in Symmetry
Omit activation related details from spec
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
2024/BIN-2024-0006.md Outdated Show resolved Hide resolved
in conjunction with `OP_CHECKSIGFROMSTACK`[^CSFS] and `OP_INTERNALKEY`[^IKEY],
making the script cleaner, and simpler. If `OP_CAT`[^CAT] was used naively, the
contract could be easily broken since `OP_CHECKTEMPLATEVERIFY`[^CTV] is only
defined for 32 byte parameters.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be confusing two things? I would have expected "Vector commitments" to be about committing to N>2 elements, via something like:

  • witness stack: [A B C] (C at top)
  • script: PAIRCOMMIT PAIRCOMMIT <PC(A, PC(B, C))> EQUALVERIFY

(That approach is no longer close-to-optimal wrt hashing overhead, so it might be worth explaining why that makes sense)

But instead you're mostly talking about how it compares to a naive use of CAT that would already be broken with N=2?

I would have thought it would be better to compare to an intelligent use of CAT, and to other approaches like Liquid's streaming SHA opcodes (which would have less hashing overhead than repeated PAIRCOMMIT) or perhaps being able to slide in additional commitments to CHECKSIG/CSFS with a POP_SIGDATA opcode.

Comparison to other approaches probably should be left for a "rationale" section particularly if Motivation is moved up prior to the Spec. I think both POP_SIGDATA and streaming SHA opcodes would allow reduced hashing overhead compared to PAIRCOMMIT`` when you're committing to more than 2 items, so would be interesting to understand why those aren't better approaches, given you're optimising the hashing overhead when N=2.

Copy link
Author

@moonsettler moonsettler Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about vector commitments, it actually is impossible to predict what would be more optimal for the user a) leave the items on the stack and only consume the number of items, or b) consume the items from the stack.

So I would probably do <vch1> .. <vchn> <n> VECTORCOMMIT where n as a signed char can be 2..127 or -2..-127, and if it's positive the stack elements are left on stack, if negative they are consumed. This would ofc be a different opcode and is not optimal for Symmetry.

SHA256 streaming could be a useful addition if we already have CAT with script restoration. I'm not sure if consensus is possible on that any time soon. In current script it would be too controversial as it naively relaxes the script limitations on what is possible both the limitation of stack element size that can get hashed with CAT only and without CAT it allows for custom construction of 'sighashes' like CTV templates or with CSFS pretty much everything CAT enables in terms of introspection.

PAIRCOMMIT is domain separated and limited on purpose, to hopefully avoid that controversy.

ELSE
<settlement-n-hash> CTV
ENDIF
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are LN-Symmetry specific examples? There's not really enough context to follow them here though, and I would have thought they'd make more sense in a separate LN/LNHANCE-Symmetry BOLT?

2024/BIN-2024-0006.md Show resolved Hide resolved
@moonsettler moonsettler marked this pull request as draft November 2, 2024 23:53
@moonsettler
Copy link
Author

Thanks for the suggestions! Will clean it up! @ajtowns

@moonsettler
Copy link
Author

Changed OP_PAIRCOMMIT implementation and specification based on feedback from @ajtowns.

@moonsettler moonsettler marked this pull request as ready for review November 8, 2024 17:25
highly optimized for just 2 stack elements.

```text
# pc-hash = PC(a, PC(b, c))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong but shouldn't this be

PC(PC(c, b), a)

If the stack is

c
b
a

And

PC(PC(a, b), c)

If the stack is

a
b
c

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PC takes the top element as vch2 and the one below the top as vch1. Then pushes PC(vch1, vch2) on stack.

    const valtype& vch1 = stacktop(-2);
    const valtype& vch2 = stacktop(-1);

    uint256 hash = PairCommitHash(vch1, vch2);

It should be

<a>
<b>
<c>
# stack: <a> <b> <c>
PC
# stack: <a> <PC(b, c)>
PC
# stack: <PC(a, PC(b, c)>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I see I had misread the code, this makes sense now thanks for clearing it up!

Copy link
Author

@moonsettler moonsettler Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the unit tests, you can execute PC only if you check out that branch, build it and run

test_bitcoin --run_test=pchash_tests

pchash_reproduce on line 68 shows you how to manually reproduce the output, and pchash_tapscript shows you how you can give it a spin in the interpreter.

https://github.com/lnhance/bitcoin/blob/lnhance-26.x/src/test/pchash_tests.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants